Conversation
janus_ice_cb_agent_closed() ignored its `src` argument (the agent whose close completed) and instead unref'd and NULLed handle->agent. When a re-offer arrives in the cleanup window, janus_ice_webrtc_free() clears the CLEANING/HAS_AGENT flags while nice_agent_close_async() is still in flight, so janus_ice_setup_local() creates a fresh agent and overwrites handle->agent before the close callback runs. The callback then unrefs and NULLs the *new* agent, on the handle's mainloop, while the requests thread holds handle->mutex inside setup_local -- the handle thread dies there, the mutex is never released, and every request on the handle blocks behind it (instance-wide API deadlock). Act on the agent the callback was handed (src) instead of re-reading handle->agent, and only clear handle->agent if it still points at the agent being closed.
|
Mh, but what would be the reproducible race you mention? We use loops for handles, which means in theory each handle is triggered by the same thread, respectively. Is the issue happening because the same |
|
You're right that each handle has its own loop/thread — but the offer that creates the new agent doesn't run on it. JSEP processing runs on the |
Is this reproducible on 0.x? |
|
Yes — same code on 0.x ( But I didn't have a chance to reproduce it. |
|
Do you have an easy way to reproduce the race? |
|
@denesdenesdenes ping 🙂 |
|
So far I did not reproduce it, the crash happened in production. |
|
Repro steps, finally. On an unmodified build: bring up a publisher PeerConnection, send a re-offer, then close the PC so the browser sends a DTLS close_notify. The re-offer parks in the CLEANING wait, janus_ice_webrtc_free() clears the flags while the async close is still pending, and janus_ice_setup_local() installs a second agent. That race is very narrow, so to hit it every time I added two sleeps — both are needed: g_usleep(200000) at the top of janus_ice_cb_agent_closed() 25 runs each. Unpatched disposed a different agent than it closed, 25/25, and leaked it — 50 agents created, 25 disposed. Patched: 0/25, and 50/50. One caveat: I couldn't wedge the instance in the lab. The stray unref doesn't reach refcount zero there, so it surfaces as a leak rather than the deadlock we hit in production. |
|
Thanks! I think it does make sense to merge this. I'll backport to |
janus_ice_cb_agent_closed() ignored its `src` argument (the agent whose close completed) and instead unref'd and NULLed handle->agent. When a re-offer arrives in the cleanup window, janus_ice_webrtc_free() clears the CLEANING/HAS_AGENT flags while nice_agent_close_async() is still in flight, so janus_ice_setup_local() creates a fresh agent and overwrites handle->agent before the close callback runs. The callback then unrefs and NULLs the *new* agent, on the handle's mainloop, while the requests thread holds handle->mutex inside setup_local -- the handle thread dies there, the mutex is never released, and every request on the handle blocks behind it (instance-wide API deadlock). Act on the agent the callback was handed (src) instead of re-reading handle->agent, and only clear handle->agent if it still points at the agent being closed.
3 participants
Summary
janus_ice_cb_agent_closed()ignores itssrcargument — the agent whoseasync close just completed — and instead operates on the current
handle->agent. Under a reproducible race this unrefs and NULLs adifferent, freshly-created agent, deadlocking the handle; because the
handle thread dies holding
handle->mutex, the instance stops answeringits API on every transport.
The race
mainloop,
janus_ice_webrtc_free()callsnice_agent_close_async(handle->agent, janus_ice_cb_agent_closed, ...)and then, before that async close completes, clears
JANUS_ICE_HANDLE_WEBRTC_CLEANINGand..._HAS_AGENT.handle->agentstill points at the closing agent (it is only NULLed later, in the callback).
CLEANINGwait injanus_process_incoming_request(), seesCLEANINGclear, proceeds, andtakes
handle->mutex.janus_ice_setup_local()findsHAS_AGENTclear, so the"Agent already exists?" guard doesn't fire, and it creates a new agent,
overwriting
handle->agent.janus_ice_cb_agent_closed()runs, reads
handle->agent— now the new agent — and unrefs + NULLsit while the requests thread holds
handle->mutexinsidesetup_local().The handle thread dies there, the mutex is never released, and every
later request on the handle blocks behind it.
Log signature:
Creating ICE agentimmediately followed byDisposing nice agent, then silence — noHandle thread ended!.The fix
Act on the agent the callback was handed (
src) instead of re-readinghandle->agent, and only clearhandle->agentif it still points at theagent being closed.
srcis the agent: libnice'snice_agent_close_async()builds its task with
g_task_new(agent, ...), so the callback's sourceobject is the closing agent.
Affected versions
Present on current
master(4602fcc) and the latest releasev1.4.1(identical code, shifted a few lines). I couldn't find an existing issue
covering it.